plan: add feature plan for DH-21375 databars integration with TableFormat #1282
Conversation
Add comprehensive guide for AI agents working in the deephaven-plugins repository covering: - Environment setup (Python and JavaScript) - Building plugins (Python wheels and JS bundles) - Running unit tests (tox) and E2E tests (Playwright) - Running development server (deephaven-core and Docker) - Documentation preview and snapshot generation - Code quality and pre-commit hooks - Common development workflows - Release management Prioritizes local development workflows over Docker-based approaches.
Add feature planning infrastructure: - Create plugins/ui/plans/ directory for feature plan documents - Add README with naming convention (ticket number + keyword) - Define plan document structure (overview, goals, design, implementation, testing, docs, open questions) - Update AGENTS.md with general instructions for creating feature plans in any plugin
…rmat Create comprehensive plan document for integrating databars into ui.TableFormat: - Align databars with other formatting features for API consistency - Enable conditional databars with if_ parameter support - Maintain backward compatibility with deprecated databars parameter - Define 5-phase implementation: foundation, JS integration, testing, docs, cleanup - Document testing strategy, migration guide, and success criteria - Address open design questions with recommendations
|
Pull Request titles must follow the Conventional Commits specification. Details: |
|
Whatever you come up with, keep in mind the heatmap function will likely share like 70% of the same props (min/max/colors[]/value_column etc.) Ideally, a single change could switch between the two. Just take that into consideration. |
| mode: Literal["databar"] | None = None # Change from TableDatabar to literal | ||
|
|
||
| # Databar-specific properties (only used when mode="databar") | ||
| value_column: ColumnName | None = None | ||
| min: ColumnName | float | None = None | ||
| max: ColumnName | float | None = None | ||
| axis: Literal["proportional", "middle", "directional"] | None = None | ||
| direction: Literal["LTR", "RTL"] | None = None | ||
| value_placement: Literal["beside", "overlap", "hide"] | None = None | ||
| opacity: float | None = None | ||
| markers: list[dict] | None = None # For marker lines |
There was a problem hiding this comment.
It looks like I previously wired this up in the Python, but didn't do the JS side for some reason (the types exist in JS though)
I have it as mode took a ui.TableDatabar which had the params. That is the type for the standalone databars though, so it still has a column prop. I think it was to not clutter the props as much and because JS uses more nested props. Not sure how "Pythonic" my version is though
One thing to consider is the format can specify multiple cols, but databars were initially designed to work with 1 column per config. So we would need to identify how we want it to work if the table format defines multiple cols and how we want to work with defining multiple databar columns
I think my initial thought was if you wanted multiple databars, you needed a list of TableFormat. Although there is the case where maybe you have constants for min/max and want multiple columns with the same min/max which could make sense in a single TableFormat.
Auto min/max with multiple cols we would need to wire up. value_column would really only make sense for "I have column A and want to display the results in B" not across multiple cols
There was a problem hiding this comment.
I also see databars as a mode of rendering whereas the other format options are all individual things applied to a cell (like rendering mode would be). With the props for databar/heatmap/any other rendering mode at the top level, we potentially run into a cluster of props that on their own do nothing and could be a little confusing. A few of the props could be mistaken as standalone (like opacity or maybe direction)
If TableDatabar and TableHeatmap or whatever we'd want to call the dataclasses had the same props, then changing from databar to heatmap would be simple.
| cols="Size", | ||
| if_="Size > 100", | ||
| mode="databar", | ||
| color=["negative", "positive"], |
There was a problem hiding this comment.
I don't think our Python type (or TS possibly) around color is correct, but for databar we would want to allow a gradient
| 1. **Should we keep `TableDatabar` dataclass?** | ||
|
|
||
| - Option A: Remove it entirely, flatten all properties into `TableFormat` | ||
| - Option B: Keep it as `mode=TableDatabar(...)` for grouping databar options | ||
| - **Recommendation**: Option A (flatten) for simplicity and consistency |
There was a problem hiding this comment.
See my comments above why I think Option B might be better. Basically there will be props that apply to databar, but not anything else or props that might apply to both. For example, color we would need to check that the mode isn't a mode which consumes the color prop and then use it as text color if we're not in a mode which uses color. But maybe we want to also set text color on top of a databar color in which case we now need a new prop
| 5. **Should we support databars without specifying columns (row-level databars)?** | ||
|
|
||
| - **Recommendation**: No, databars are inherently column-specific |
There was a problem hiding this comment.
This could have some use with explicit min/max like a matrix of values and you want to see a databar 0-100
3 participants
@gzh2003 @mattrunyon take a look at the proposal in the DH-21375 planning document as part of this PR and see if you have any comments about it. Can try playing with databars as they are currently to help inform your opinion: https://deephaven.io/core/ui/docs/components/table/#formatting-databars
Create comprehensive plan document for integrating databars into ui.TableFormat: